Skip to content

Ensure commit sha is always passed to Message#24

Open
tobischo wants to merge 3 commits intoprontolabs:masterfrom
tobischo:fix/return-commit-sha-in-message-correctly
Open

Ensure commit sha is always passed to Message#24
tobischo wants to merge 3 commits intoprontolabs:masterfrom
tobischo:fix/return-commit-sha-in-message-correctly

Conversation

@tobischo
Copy link
Copy Markdown

In #18 the issue is raised the gitlab API cannot be correctly addressed with the gem in the state it is right now.

This seems to be happening because the commit sha is set to nil in any case

Message.new(path, line, level, offence['message'], nil, self.class)

Lines would provide the commit sha correctly for the change and the error caused by it, but those are only accessed for non fatal errors.

The commit sha is also stored at the top level @patches variable which can provide it for the commit that was last pushed/on which the check is running. In for fatal errors it is possible then to use this commit message to attach the messages

@tobischo tobischo requested a review from a team as a code owner May 10, 2021 15:27
@ashkulz
Copy link
Copy Markdown
Member

ashkulz commented May 11, 2021

I'm not sure about this change, as I looked at pronto-rubocop and pronto-haml and it has the same logic -- so if it's a bug, it's more wide-spread than this linter. Can you reproduce it with other linters?

@tobischo
Copy link
Copy Markdown
Author

tobischo commented May 11, 2021

You are right, for pronto-rubocop it is not there and it currently is also not causing issues for us.

I know that I added it for pronto-golang

But let's go through the code:

The runners are executed here:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto.rb#L66
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/runners.rb#L20

We are using the -f gitlab option, so
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/formatter.rb#L19

And the gitlab formatter
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/gitlab_formatter.rb#L3
is coming from the commit formatter
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/commit_formatter.rb#L10
which is coming from
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/git_formatter.rb#L3

And the messages are mapped to comments here:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/git_formatter.rb#L74

and the commit_sha is coming from the message when mapping it:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/git_formatter.rb#L65

And the commit_sha in the message, if left to nil, needs to come from the line:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/message.rb#L18

For the fatal errors it therefore needs to get the commit_sha but it does not need it for other types of errors, so we could change it to not pass in the commit_sha for the places where we have a line object.
That's probably also why we do not need it for pronto-rubocop and pronto-haml.

@ashkulz
Copy link
Copy Markdown
Member

ashkulz commented May 12, 2021

Thanks for the detailed analysis, @tobischo! I'll try to take a look at it over the weekend 👍

@tobischo
Copy link
Copy Markdown
Author

Hey @ashkulz
did you maybe have some time in between to take a look at this change?

@istisiki
Copy link
Copy Markdown

Can confirm this is still a problem.
I've added pronto-eslint to our Github Actions recently and it's gives the same error.

/opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/octokit-4.21.0/lib/octokit/response/raise_error.rb:14:in `on_complete': POST https://api.github.com/repos/ORG_NAME/REPO_NAME/commits//comments: 404 - Not Found // See: https://docs.github.com/rest (Octokit::NotFound)
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/faraday-1.8.0/lib/faraday/middleware.rb:19:in `block in call'
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/faraday-1.8.0/lib/faraday/response.rb:59:in `on_complete'
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/faraday-1.8.0/lib/faraday/middleware.rb:18:in `call'
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/octokit-4.21.0/lib/octokit/middleware/follow_redirects.rb:73:in 
	...

The runners we are using are:

  • pronto-rubocop
  • pronto-eslint

Removing pronto-eslint resolves the issue for me but I would really want to have that in our CI.

@leonardoalemax
Copy link
Copy Markdown

this will solve my problem.

@ashkulz
Copy link
Copy Markdown
Member

ashkulz commented Jan 4, 2022

Sorry, this got lost in my to do list. I'll take a look at it in the evening.

@tobischo
Copy link
Copy Markdown
Author

@ashkulz Gentle reminder for this 🙂

@ashkulz
Copy link
Copy Markdown
Member

ashkulz commented Jun 29, 2022

@tobischo thanks for the reminder, I'll review this on the weekend. Sorry for the delay 🙈

@tobischo
Copy link
Copy Markdown
Author

Maybe this weekend? 😄

@tobischo
Copy link
Copy Markdown
Author

tobischo commented Aug 6, 2022

Or this one? 😉

@ashkulz
Copy link
Copy Markdown
Member

ashkulz commented Aug 13, 2022

Sorry, there's been a lot of sickness in sequence in the family. I'll do it this weekend, I promise 🙈

@tobischo
Copy link
Copy Markdown
Author

No need to explain or apologize.
As far as I am concerned I am basically asking you to spend your spare time for free on something for me.

Therefore, no rush, just gentle reminders as I also know how easily something like this can be forgotten about

@tobischo tobischo force-pushed the fix/return-commit-sha-in-message-correctly branch from c7d9be8 to 2f39674 Compare June 2, 2023 10:26
@tobischo
Copy link
Copy Markdown
Author

tobischo commented Jun 2, 2023

Alright, I resolved the conflict that got in from another change in the meantime.

Gentle reminder for a review as well

@tobischo
Copy link
Copy Markdown
Author

It's been a while 😄
Any chance you might find time in the near future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants